-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(predicate): Add has_workflow_result
predicate
#794
Conversation
Thanks for your interest in palantir/policy-bot, @iainlane! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
359e20d
to
f1f794b
Compare
f1f794b
to
d0d510d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for proposing this. I haven't used Actions enough to run into this problem, but it seems reasonable to have an Actions-specific predicate to simplify things.
Since this contains some overlap with #789, my comment from there about the allowedConclusions
set also applies here. I think we'll want to pick one of these PRs to focus on and merge first, and then update the other one to resolve conflicts.
Cheers. I'll mark this as a draft until #789 is good to go, and then pivot over here, so we don't have too much in flight at once. |
f346b0e
to
8875233
Compare
f9b394a
to
01ee4dd
Compare
480e662
to
49bf099
Compare
49bf099
to
fd32004
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for adding tests for both the predicate and the pull.Context
method. I think you missed registering the new event handler with the server, but should be good to merge once that's in place.
"github.com/pkg/errors" | ||
) | ||
|
||
type WorkflowRun struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll also need to register this new handler with the EventDispatcher
so it will actually get the events: https://github.com/palantir/policy-bot/blob/develop/server/server.go#L184-L192
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, thanks! Here it is.
I suppose it mostly worked for us because you tend to get check_run
events anyway...
This is a new predicate which matches on the results of an entire GitHub Actions workflow. This is similar to `has_status`, except that matches on statuses only, which for GitHub Actions are roughly equivalent to _jobs_ in a Workflow. It's often convenient to match on every job. The workflows are given as paths in the repository, as these can't be dynamic and are easy to predict when writing policies.
fd32004
to
af8fa99
Compare
This is a new predicate which matches on the results of an entire GitHub Actions workflow.
This is similar to (doesn't replace, both are complementary)
has_status
from #789, excepthas_status
matches on statuses or check runs, which for GitHub Actions are basically equivalent to jobs in a Workflow.has_workflow_result
finds the check suite for the PR and reports its conclusion, which is the conclusion of the whole workflow run - of all of its jobs.With
has_workflow_result
, the workflows are given as paths in the repository, as these can't be dynamic and are easy to predict when writing policies, as well as easy to review for correctness.With this, you can write
and then we'll wait for all the jobs in this workflow to succeed (or with the optional
conclusions
, to conclude with whatever you want), which is more convenient than listing all the job names out. This new predicate is really to make writing rules easier. At least for the workflows I use daily it's much more common to want to wait for every job than just a subset of them. 🙂For the avoidance of doubt, it doesn't solve the problem I mentioned in #789 where we don't see completely skipped jobs (if the
on
filters resulted in a skip).